Repository navigation
fix: treat a dropped parallel cache write as a cache miss - #22289
Closed
DawnofGenX wants to merge 1 commit into
Closed
DawnofGenX wants to merge 1 commit into
DawnofGenX wants to merge 1 commit into
Conversation
A parallel worker can find a module's cache entry missing when the write was dropped, e.g. because the sqlite cache was locked by a concurrent mypy run (python#22241): SqliteMetadataStore.write() returns False and write_cache tolerates that. Two places turned that tolerated condition into an assertion: - State.reload_meta() asserted that the meta exists. It now keeps the interface hash it already has, so dependents revalidate against the cache. - State.load_tree() left self.tree as None when the data file was missing, which crashed later in fix_cross_refs(). A parallel worker now parses the source instead, i.e. treats the dropped write as a cache miss. The sequential path is unchanged.
Contributor
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #22241
A parallel worker can find a module's cache entry missing when the cache write was dropped:
SqliteMetadataStore.write()returnsFalseonsqlite3.OperationalError("database is locked" after the busy timeout) andwrite_cachedeliberately tolerates that, as sequential mypy does. In-nmode two places turned that tolerated condition into an assertion:State.reload_meta()asserted that the meta exists. It now keeps the interface hash it already has, so dependents revalidate against the cache. Its only caller (maybe_load_deps) is already guarded byparallel_worker, so the sequential path is untouched.State.load_tree()leftself.treeasNonewhen the data file was missing, which crashed later infix_cross_refs()("method must be called on parsed file only"). A parallel worker now parses the source instead — a dropped cache write is a cache miss, not an internal error. Without this second half the first one only moved the crash downstream, which is why both are here.Verification
Reproduced first: a two-process harness (a helper holds the sqlite write lock for 7s while
mypy -n 4 --cache-dir cacheruns) produces the reported crash on pre-fix code — three workers fail withassert new_meta_pair is not None(INTERNAL ERROR). A clean parallel run with no lock succeeds 5/5, so the crash comes from the dropped write.Each half has a regression test that bites:
test_reload_meta_missing_cache_is_noop: with a cache directory that has no meta for the module, restoring the oldassert new_meta_pair is not Nonemakes this test fail withAssertionError; with the fix the existing interface hash is kept.test_load_tree_parses_when_cache_data_is_missing: with a real source file and an empty cache directory, removing only theload_tree()fallback makes this test fail (state.treestaysNone); with the fix the source is parsed.Both tests use a real temporary cache directory and a real source file instead of
mock.patchonmypy.build: patching module-level functions has no effect in mypyc-compiled builds (globals are resolved statically there), so a mock-based version passes interpreted and fails compiled.On this PR's base (
upstream/masterat the time of writing):pytest mypy/test/testgraph.py→ 14 passed;pytest mypy/test/testcheck.py -k "parallel or cache"→ 49 passed, 3 skipped.ruff 0.14.3 checkandblack 26.1.0 --checkare clean on both changed files.Scope
The dropped write can leave the meta present but the data file missing, or vice versa; both are now treated as cache misses. This does not change the cache format, and it does not make concurrent runs that share one cache directory supported (#13916) — it removes the assertions that turned a tolerated condition into a crash.
Not run here: the two-
mypy -n-processes variant from the issue (the single-process + lock-holder harness triggers the same dropped-write path).